Replace sun.misc.Unsafe used in LazyVals with VarHandles #24109

jchyb · 2025-09-29T17:20:42Z

~~Just a few small tweaks left:~~

~~extract MethodHandles.lookup into local variable in the static constructor~~

~~[x] maybe inline objCAS2 (should give us more freedom, allow us to backport into 3.3 in the worst case scenario (hopefully not))~~
EDIT: done

This PR replaces the use of the deprecated Unsafe with VarHandles, available in java 9+. Important thing about VarHandles is that for private members (like our val dictating the value/resolution progress of the lazy val), the findVarHandle method must be called from the class containing the val, otherwise we end up with an error. This meant that some custom logic had to be added to the MoveStatics phase, which would usually move static constructor to the companion object (which we do not want for the VarHandles here).
Only the newer implementation was adjusted, the one available under -Ylegacy-lazy-vals was left untouched.

~~This PR also effectively drops support for java 8, with java 17 being the new required version. As part of that:~~

lowest supported version for -release/-java-target-version is now 17 (earlier versions will throw an error). -Xunchecked-java-output-version was left untouched.

managed community-build tests are now run with java 17. Options setting -release 8 were removed for projects that were using that. Projects that were using javax.xml.bind (dropped in java 11) were removed altogether (playJson and betterfiles)

~~Java version used for releases was set to 17~~ (Added separately in Require JDK 17+ #24146)

sjrd · 2025-10-03T08:02:53Z

extract MethodHandles.lookup into local variable in the static constructor

I actually wonder: is that worth it? It seems to bring additional complexity to the transformation. Sure, we need one call for every lazy val in the class, but does it matter? It's in the static initializer anyway, which is executed O(1) times.

maybe inline objCAS2

IMO that is really worth it, but for a different reason: it removes the last reference to the LazyVals$ module class in the generated code. That's good because we are then reasonably sure that we won't hit load-class-time warnings about the Unsafe val in that module class. AFAICT, generating the inlined code would not be any harder than generating the call to objCAS2. It's one method call.

(We can drop the if (debug) thing at this point anyway.)

jchyb · 2025-10-03T09:57:44Z

extract MethodHandles.lookup into local variable in the static constructor

I actually wonder: is that worth it? It seems to bring additional complexity to the transformation. Sure, we need one call for every lazy val in the class, but does it matter? It's in the static initializer anyway, which is executed O(1) times.

It's an obvious improvement so I wanted to take it. Unfortunately, even without this we still had to introduce complexity in the MoveStatics phase (directly linking to the LazyVals phase), so we are not doing that much more (1st commit vs 2nd commit). I'll try running some benchmark tests to see if it's worth it.

maybe inline objCAS2

IMO that is really worth it, but for a different reason: it removes the last reference to the LazyVals$ module class in the generated code. That's good because we are then reasonably sure that we won't hit load-class-time warnings about the Unsafe val in that module class. AFAICT, generating the inlined code would not be any harder than generating the call to objCAS2. It's one method call.

(We can drop the if (debug) thing at this point anyway.)

Right, I hadn't even thought about that (we still reference the nested classes but now I realize that's not an issue). Weirdly, even with the objCAS2 call, the warnings disappeared for me.

jchyb · 2025-10-07T09:19:16Z

@sjrd Issues with CI took me a little more time than I anticipated, so I haven't done the promised benchmarks yet. Still I am willing to remove the initialization optimization if it complicates things too much.

I did test whether this works with native image though (it does).

sjrd

Nice work overall!

sjrd · 2025-10-07T14:58:23Z

compiler/src/dotty/tools/dotc/config/ScalaSettingsProperties.scala

  private lazy val minTargetVersion = classfileVersionMap.keysIterator.min
  private lazy val maxTargetVersion = classfileVersionMap.keysIterator.max

+  private val minReleaseVersion = 17


Shouldn't we change classfileVersionMap instead?

sjrd · 2025-10-07T14:58:38Z

compiler/src/dotty/tools/dotc/config/ScalaSettingsProperties.scala

    (minTargetVersion to maxTargetVersion).toList.map(_.toString)

  def supportedReleaseVersions: List[String] =
    if scala.util.Properties.isJavaAtLeast("9") then


This is true now.

sjrd · 2025-10-07T15:11:04Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+    val varHandleInfo = appendVarHandleDefs.getOrElseUpdate(claz, new VarHandleInfo(EmptyTree, Nil))
+    varHandleInfo.methodHandlesLookupDef match
+      case EmptyTree =>
+        val lookupSym: TermSymbol = newSymbol(claz, (s"${claz.name}${lazyNme.methodHandleLookupSuffix}").toTermName, Synthetic, defn.MethodHandlesLookupClass.typeRef).enteredAfter(this)


Turning names into strings like that is not recommended. Consider something like

Suggested change

val lookupSym: TermSymbol = newSymbol(claz, (s"${claz.name}${lazyNme.methodHandleLookupSuffix}").toTermName, Synthetic, defn.MethodHandlesLookupClass.typeRef).enteredAfter(this)

val lookupSym: TermSymbol = newSymbol(claz, claz.name.toTermName.withSuffix(lazyNme.methodHandleLookupSuffix), Synthetic, defn.MethodHandlesLookupClass.typeRef).enteredAfter(this)

sjrd · 2025-10-07T15:12:15Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+      case _ =>
+
+    // create a VarHandle for this lazy val
+    val varHandleSymbol: TermSymbol = newSymbol(claz, s"$containerName${lazyNme.lzyHandleSuffix}".toTermName, Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this)


Same here:

Suggested change

val varHandleSymbol: TermSymbol = newSymbol(claz, s"$containerName${lazyNme.lzyHandleSuffix}".toTermName, Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this)

val varHandleSymbol: TermSymbol = newSymbol(claz, containerName.toTermName.withSuffix(lazyNme.lzyHandleSuffix), Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this)

Also, can't we make those symbols Private?

sjrd · 2025-10-07T15:20:11Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

  }
+
+  extension (sym: Symbol) def isVarHandleForLazy(using Context) =
+    sym.name.endsWith(lazyNme.lzyHandleSuffix)


Looks like we should use a dedicated NameKind if we need to do that kind of test.

sjrd · 2025-10-07T15:23:14Z

compiler/src/dotty/tools/dotc/transform/MoveStatics.scala


-            val staticAssigns = staticFields.map(x => Assign(ref(x.symbol), x.rhs.changeOwner(x.symbol, staticCostructor)))
-            tpd.DefDef(staticCostructor, Block(staticAssigns, tpd.unitLiteral)) :: newBody
+            val symbolRemap = localStaticDefs.map { x =>


IMO this is too complex to introduce without a very good reason.

I suggest we stick to the simpler alternative where we repeat the calls to lookup().

sjrd · 2025-10-07T15:25:01Z

compiler/src/dotty/tools/dotc/transform/MoveStatics.scala

+              localStaticDefs.addOne(valDef)
+            case memberDef: MemberDef if memberDef.symbol.isScalaStatic =>
+              if memberDef.symbol.isVarHandleForLazy then
+                staticTiedDefs.addOne(memberDef)


It looks like this could be

Suggested change

staticTiedDefs.addOne(memberDef)

remainingDefs.addOne(memberDef)

which would remove the need for staticTiedDefs altogether.

sjrd · 2025-10-07T15:25:37Z

presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionRelease8Suite.scala

 import dotty.tools.pc.utils.JRE

-class CompletionRelease8Suite extends BaseCompletionSuite:
+@Ignore class CompletionRelease8Suite extends BaseCompletionSuite:


Looks like this still needs to be addressed?

It doesn't work anymore, because the -release 8 option throws an error now (on purpose). I'll add the comment explaining that. If it's clearer to just remove the test completely I can do that too

I see. Yes, in that case we should remove the test entirely.

Perhaps extract a PR that really gets rid of JDK 8 support. There seem to be several commits about that in this PR, and they should stand on their own.

sjrd · 2025-10-07T15:27:34Z

sbt-test/sbt-dotty/scaladoc-regressions/build.sbt

 lazy val i18231 = project
  .in(file("i18231"))
-  .settings(scalacOptions += "-release:8")
+  // .settings(scalacOptions += "-release:8")


Doesn't that invalidate the purpose of the test? Should it be retargeted to 17?

This is paving the way for #24109. As part of this: * the minimum version for `-release`/`-java-output-version` flag is set to 17 * managed community build tests are done on JDK 17 * some community-build projects are temporarily removed * some that were using -release 8 option now have that option removed * compilation tests using `-release 8` were disabled * presentation compiler tests using `-release 8` and `-release 11` were removed * releases are done on JDK 17

jchyb force-pushed the lazyval-varhandle branch 5 times, most recently from a82a297 to 5aafc89 Compare October 2, 2025 14:32

jchyb force-pushed the lazyval-varhandle branch 6 times, most recently from 9032701 to bb1b2ca Compare October 6, 2025 18:07

jchyb marked this pull request as ready for review October 7, 2025 09:15

jchyb requested a review from sjrd October 7, 2025 09:15

sjrd requested changes Oct 7, 2025

View reviewed changes

jchyb self-assigned this Oct 7, 2025

jchyb mentioned this pull request Oct 7, 2025

Require JDK 17+ #24146

Merged

jchyb force-pushed the lazyval-varhandle branch 2 times, most recently from c578c64 to 045cec4 Compare October 8, 2025 12:06

Replace depracated Unsafe with VarHandles in the lazy val implementation

aa26211

jchyb force-pushed the lazyval-varhandle branch from 045cec4 to aa26211 Compare October 8, 2025 12:11

jchyb requested a review from sjrd October 9, 2025 09:51

	val lookupSym: TermSymbol = newSymbol(claz, (s"${claz.name}${lazyNme.methodHandleLookupSuffix}").toTermName, Synthetic, defn.MethodHandlesLookupClass.typeRef).enteredAfter(this)
	val lookupSym: TermSymbol = newSymbol(claz, claz.name.toTermName.withSuffix(lazyNme.methodHandleLookupSuffix), Synthetic, defn.MethodHandlesLookupClass.typeRef).enteredAfter(this)

	val varHandleSymbol: TermSymbol = newSymbol(claz, s"$containerName${lazyNme.lzyHandleSuffix}".toTermName, Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this)
	val varHandleSymbol: TermSymbol = newSymbol(claz, containerName.toTermName.withSuffix(lazyNme.lzyHandleSuffix), Synthetic, defn.VarHandleClass.typeRef).enteredAfter(this)

	staticTiedDefs.addOne(memberDef)
	remainingDefs.addOne(memberDef)

Replace sun.misc.Unsafe used in LazyVals with VarHandles #24109

Are you sure you want to change the base?

Replace sun.misc.Unsafe used in LazyVals with VarHandles #24109

Conversation

jchyb commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjrd commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jchyb commented Oct 3, 2025

Uh oh!

jchyb commented Oct 7, 2025

Uh oh!

sjrd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jchyb commented Sep 29, 2025 •

edited

Loading

sjrd commented Oct 3, 2025 •

edited

Loading